Add salt generation to demo security configuration by cwperks · Pull Request #6022 · opensearch-project/security

cwperks · 2026-03-19T13:35:37Z

Description

The changes in this PR more strongly encourage a cluster administrator to configure all aspects of security including the salt used for Field Masking.

Currently, when using the demo security configuration we log out a warning message when left uncustomized

[WARN ] 2024-01-24 10:56:14.205 [Test worker] Salt - If you plan to use field masking pls configure compliance salt e1ukloTsQlOgPquJ to be a random string of 16 chars length identical on all nodes

Even if plugins.security.allow_unsafe_democertificates is set to false the cluster boots up. This PR introduces code to enforce that this value is customized, but leaves it unwired as it could break existing deployments. I propose that this would be enforced in next major release.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Maintenance

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

codecov · 2026-03-19T15:26:30Z

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.77%. Comparing base (ea6087c) to head (e15987a).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/org/opensearch/security/configuration/Salt.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6022      +/-   ##
==========================================
- Coverage   74.78%   74.77%   -0.01%     
==========================================
  Files         447      447              
  Lines       28467    28481      +14     
  Branches     4328     4332       +4     
==========================================
+ Hits        21289    21298       +9     
- Misses       5184     5186       +2     
- Partials     1994     1997       +3

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`84.91% <ø> (ø)`
.../org/opensearch/security/support/ConfigHelper.java	`88.23% <ø> (ø)`
...y/tools/democonfig/SecuritySettingsConfigurer.java	`73.40% <100.00%> (+1.02%)`	⬆️
...va/org/opensearch/security/configuration/Salt.java	`96.15% <80.00%> (-3.85%)`	⬇️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Craig Perkins <cwperx@amazon.com>

github-actions · 2026-04-28T18:54:26Z

PR Reviewer Guide 🔍

(Review updated until commit `1b0b971`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [522]: Uncomment for 4.0 - enforce that the default compliance salt is not used outside of demo configuration
🔀 Multiple PR themes Sub-PR theme: Refactor test helpers to use FieldMaskingTestHelper.DEFAULT Relevant files: src/integrationTest/java/org/opensearch/security/privileges/dlsfls/FieldMaskingTestHelper.java src/integrationTest/java/org/opensearch/security/privileges/dlsfls/FieldMaskingTest.java src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DlsFlsLegacyHeadersTest.java src/integrationTest/java/org/opensearch/security/privileges/dlsfls/FlsDocumentFilterTest.java Sub-PR theme: Add salt validation logic for production enforcement Relevant files: src/main/java/org/opensearch/security/configuration/Salt.java src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java src/test/java/org/opensearch/security/configuration/SaltTest.java Sub-PR theme: Generate random salt in demo security configuration Relevant files: src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java src/main/java/org/opensearch/security/support/ConfigHelper.java
⚡ Recommended focus areas for review Salt Character Set The `SALT_CHARS` includes special characters like `!@#$%^&`. If the generated salt is written to a YAML config file, certain special characters may need escaping or could cause YAML parsing issues. Validate that all characters in the set are safe for YAML values without quoting. static final String SALT_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!@#$%^&"; Commented-Out Enforcement The `validateSaltSettings` method is fully implemented and tested but intentionally left commented out in `OpenSearchSecurityPlugin.java`. This means the enforcement is never actually triggered, making the new validation code dead code until manually uncommented. The PR description acknowledges this, but reviewers should confirm the TODO comment and the plan for the next major release are clearly tracked. public static void validateSaltSettings(final Settings settings) { final String saltAsString = settings.get( ConfigConstants.SECURITY_COMPLIANCE_SALT, ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT ); final boolean allowUnsafeDemoCertificates = settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_UNSAFE_DEMOCERTIFICATES, false); if (ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT.equals(saltAsString) && !allowUnsafeDemoCertificates) { throw new OpenSearchException( "Default compliance salt is not allowed in production. Please configure " + ConfigConstants.SECURITY_COMPLIANCE_SALT + " to a random 16-character string, or set " + ConfigConstants.SECURITY_ALLOW_UNSAFE_DEMOCERTIFICATES + " to true for demo/test environments." ); } } Weak Salt Validation The test only checks that the salt key is present and its length is 16. It does not verify that the generated salt is actually random (i.e., different on each call) or that it only contains characters from `SALT_CHARS`. Consider adding a test that calls `generateRandomSalt()` twice and asserts the values differ, and one that validates the character set. assertThat(actual.containsKey(ConfigConstants.SECURITY_COMPLIANCE_SALT), equalTo(true)); assertThat(((String) actual.get(ConfigConstants.SECURITY_COMPLIANCE_SALT)).length(), equalTo(16));

github-actions · 2026-04-28T18:54:55Z

PR Code Suggestions ✨

Latest suggestions up to 1b0b971

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid YAML-unsafe special characters in salt The `SALT_CHARS` string contains special characters like `!@#$%^&` that may cause issues when written to YAML configuration files, as some of these characters have special meaning in YAML and may require escaping or quoting. Consider restricting the character set to alphanumeric characters only to ensure safe YAML serialization. src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java [50] -static final String SALT_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!@#$%^&"; +static final String SALT_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; Suggestion importance[1-10]: 6 __ Why: Special characters like `!@#$%^&*` in `SALT_CHARS` could cause YAML serialization issues when written to `opensearch.yml`. Restricting to alphanumeric characters is a valid safety concern, though the YAML writer may handle quoting automatically.	Low
General	Validate custom salt length requirement The validation only checks if the salt equals the default value, but does not validate that a custom salt meets the required length (16 characters). A user could configure a salt that is too short or too long, which could lead to security issues. Add a length check for custom salt values. src/main/java/org/opensearch/security/configuration/Salt.java [101-109] if (ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT.equals(saltAsString) && !allowUnsafeDemoCertificates) { throw new OpenSearchException( "Default compliance salt is not allowed in production. Please configure " + ConfigConstants.SECURITY_COMPLIANCE_SALT + " to a random 16-character string, or set " + ConfigConstants.SECURITY_ALLOW_UNSAFE_DEMOCERTIFICATES + " to true for demo/test environments." ); } +if (!ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT.equals(saltAsString) && saltAsString.length() != 16) { + throw new OpenSearchException( + "Compliance salt must be exactly 16 characters. Please reconfigure " + + ConfigConstants.SECURITY_COMPLIANCE_SALT + + "." + ); +} Suggestion importance[1-10]: 4 __ Why: Adding a length check for custom salt values is a reasonable improvement, but the existing `Salt` constructor likely already handles length validation. This is a minor enhancement that goes beyond the PR's scope of just rejecting the default salt.	Low
General	Explicitly set deterministic salt for test assertions Using `Settings.EMPTY` to create the `DEFAULT` config means the default compliance salt will be used, which is the same behavior as `FieldMasking.Config.DEFAULT`. The tests that assert specific hash values (e.g., `"c042e214a8b49561577445be44c188a8e6274006b36cd0c6fba5312253cf9293"`) rely on a deterministic salt. Ensure this is intentional and that the salt used here matches what the hardcoded expected hash values were computed with. src/integrationTest/java/org/opensearch/security/privileges/dlsfls/FieldMaskingTestHelper.java [18] -public static final FieldMasking.Config DEFAULT = FieldMasking.Config.fromSettings(Settings.EMPTY); +public static final FieldMasking.Config DEFAULT = FieldMasking.Config.fromSettings( + Settings.builder() + .put(ConfigConstants.SECURITY_COMPLIANCE_SALT, ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT) + .build() +); Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify that `Settings.EMPTY` produces the same default salt as `FieldMasking.Config.DEFAULT`, which is already the case since `fromSettings(Settings.EMPTY)` falls back to `SECURITY_COMPLIANCE_SALT_DEFAULT`. The `improved_code` is functionally equivalent to the existing code, making this a low-impact suggestion.	Low

Previous suggestions

Suggestions up to commit 1b0b971

Category	Suggestion	Impact
Possible issue	Avoid YAML-unsafe special characters in salt The `SALT_CHARS` string contains special characters like `!@#$%^&` which may cause issues when the salt is written to YAML configuration files, as some of these characters have special meaning in YAML and may require escaping. Consider restricting the character set to alphanumeric characters only to avoid potential parsing issues. src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java [50] -static final String SALT_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!@#$%^&"; +static final String SALT_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; Suggestion importance[1-10]: 6 __ Why: Special characters like `!@#$%^&*` in `SALT_CHARS` could cause YAML parsing issues when written to `opensearch.yml`. Restricting to alphanumeric characters is a valid safety concern, though the salt value is likely written as a quoted string which may mitigate the issue.	Low
General	Assert generated salt differs from default The test only verifies that the salt key is present and has length 16, but does not verify that the salt value is not equal to the default salt (`ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT`). This is the core security requirement being introduced, and it should be explicitly tested. src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java [265-266] assertThat(actual.containsKey(ConfigConstants.SECURITY_COMPLIANCE_SALT), equalTo(true)); assertThat(((String) actual.get(ConfigConstants.SECURITY_COMPLIANCE_SALT)).length(), equalTo(16)); +assertThat(actual.get(ConfigConstants.SECURITY_COMPLIANCE_SALT), not(equalTo(ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT))); Suggestion importance[1-10]: 5 __ Why: Adding an assertion that the generated salt is not equal to `SECURITY_COMPLIANCE_SALT_DEFAULT` would explicitly test the core security requirement. This is a valid test improvement that directly validates the intended behavior.	Low
General	Validate minimum salt length requirement The validation only checks for the exact default salt value, but does not validate that a configured salt meets the minimum length requirement of 16 characters. A user could configure a salt that is too short, which would silently pass validation but result in a weakened security configuration. Consider adding a length check for any configured salt value. src/main/java/org/opensearch/security/configuration/Salt.java [101-109] if (ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT.equals(saltAsString) && !allowUnsafeDemoCertificates) { throw new OpenSearchException( "Default compliance salt is not allowed in production. Please configure " + ConfigConstants.SECURITY_COMPLIANCE_SALT + " to a random 16-character string, or set " + ConfigConstants.SECURITY_ALLOW_UNSAFE_DEMOCERTIFICATES + " to true for demo/test environments." ); } +if (saltAsString.length() < 16) { + throw new OpenSearchException( + "Compliance salt must be at least 16 characters. Please configure " + + ConfigConstants.SECURITY_COMPLIANCE_SALT + + " to a random 16-character string." + ); +} Suggestion importance[1-10]: 4 __ Why: Adding a length check is a reasonable security improvement, but this is an enhancement beyond the PR's scope. The existing `Salt` constructor already handles length validation, so this would be partially redundant.	Low

Suggestions up to commit e15987a

Category	Suggestion	Impact
Possible issue	Ensure test helper salt matches expected hash values `FieldMasking.Config.fromSettings(Settings.EMPTY)` will use `ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT` as the salt, which is the same value as the old `FieldMasking.Config.DEFAULT`. The tests that assert specific hash values (e.g., `"c042e214a8b49561577445be44c188a8e6274006b36cd0c6fba5312253cf9293"`) depend on this salt being stable and matching the default. This is correct only if `fromSettings(Settings.EMPTY)` produces the same config as `FieldMasking.Config.DEFAULT`; if not, those hardcoded hash assertions will fail. Confirm that `fromSettings(Settings.EMPTY)` is equivalent to `FieldMasking.Config.DEFAULT` to avoid silent test breakage. src/integrationTest/java/org/opensearch/security/privileges/dlsfls/FieldMaskingTestHelper.java [18] -public static final FieldMasking.Config DEFAULT = FieldMasking.Config.fromSettings(Settings.EMPTY); +public static final FieldMasking.Config DEFAULT = FieldMasking.Config.fromSettings( + Settings.builder() + .put(ConfigConstants.SECURITY_COMPLIANCE_SALT, ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT) + .build() +); Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern: if `FieldMasking.Config.fromSettings(Settings.EMPTY)` doesn't produce the same salt as `FieldMasking.Config.DEFAULT`, hardcoded hash assertions in tests will fail. Making the salt explicit via `ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT` ensures the test helper is unambiguous and stable regardless of `fromSettings` implementation details.	Low
Possible issue	Prevent generated salt from matching the default value The `Salt` class validates that the salt is exactly 16 bytes (UTF-8). Special characters like `!@#$%^&` are single-byte in UTF-8, so this is fine, but the generated salt should be validated against the same constraints used by `Salt` (16 bytes). More critically, the test only checks `length() == 16` (char count), but if multi-byte characters were ever added to `chars`, this would silently break. Consider also verifying the generated salt does not accidentally equal `ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT` to avoid the validation rejection at startup. src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java [358-366] -final String chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!@#$%^&"; -SecureRandom random = new SecureRandom(); -StringBuilder sb = new StringBuilder(16); -for (int i = 0; i < 16; i++) { - sb.append(chars.charAt(random.nextInt(chars.length()))); +static String generateRandomSalt() { + final String chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!@#$%^&*"; + SecureRandom random = new SecureRandom(); + String salt; + do { + StringBuilder sb = new StringBuilder(16); + for (int i = 0; i < 16; i++) { + sb.append(chars.charAt(random.nextInt(chars.length()))); + } + salt = sb.toString(); + } while (salt.equals(ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT)); + return salt; } -return sb.toString(); Suggestion importance[1-10]: 4 __ Why: The suggestion adds a loop to ensure the generated salt never equals `ConfigConstants.SECURITY_COMPLIANCE_SALT_DEFAULT`, which would cause `validateSaltSettings` to throw. However, the probability of a random 16-char salt matching the specific default is astronomically small, making this a very marginal improvement. The improved code is accurate and logically sound.	Low

finnegancarroll · 2026-04-29T17:14:27Z

LGTM. Looks like link checker was confused by the github outage the other day.

Is there a way to track changes like this which need some enablement on the next major version? Maybe a board or label?

cwperks · 2026-04-29T17:17:57Z

Is there a way to track changes like this which need some enablement on the next major version? Maybe a board or label?

Let's start using the 4.0 label on GH issues to track anything that should be done for the next major version.

cwperks · 2026-04-29T17:28:07Z

@finnegancarroll I created #6124 for tracking

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks · 2026-04-30T19:10:40Z

@finnegancarroll @willyborankin I pushed another commit to address the comments. Can you review again?

github-actions · 2026-04-30T19:11:29Z

Persistent review updated to latest commit 1b0b971

github-actions · 2026-04-30T19:11:36Z

Persistent review updated to latest commit 1b0b971

Add salt generation to demo security configuration

aabda39

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks requested review from DarshitChanpura, RyanL1997, derek-ho, nibix, reta, shikharj05 and willyborankin as code owners March 19, 2026 13:35

cwperks added 5 commits March 19, 2026 09:55

Fix where enforcement happens

4e08fd7

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Rename

33553d7

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Add to CHANGELOG

f878231

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Fix tests

736cf28

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Only start imposing in next major release

d025b26

Signed-off-by: Craig Perkins <cwperx@amazon.com>

Merge branch 'main' into demo-salt

afd4178

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks requested a review from finnegancarroll as a code owner April 24, 2026 19:09

cwperks mentioned this pull request Apr 24, 2026

[BUG] warning about default FLS salt on startup #6105

Open

cwperks added 2 commits April 28, 2026 06:36

Merge branch 'main' into demo-salt

880c5bd

Fix compile

e15987a

Signed-off-by: Craig Perkins <cwperx@amazon.com>

finnegancarroll previously approved these changes Apr 29, 2026

View reviewed changes

cwperks mentioned this pull request Apr 29, 2026

Enforce custom compliance salt in OpenSearch 4.0 #6124

Open

3 tasks

willyborankin reviewed Apr 30, 2026

View reviewed changes

Comment thread src/main/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurer.java Outdated

Comment thread src/main/java/org/opensearch/security/configuration/Salt.java Outdated

Address comments

1b0b971

Signed-off-by: Craig Perkins <cwperx@amazon.com>

cwperks dismissed finnegancarroll’s stale review via 1b0b971 April 30, 2026 19:09

willyborankin approved these changes Apr 30, 2026

View reviewed changes

finnegancarroll approved these changes Apr 30, 2026

View reviewed changes

cwperks merged commit 651e72c into opensearch-project:main Apr 30, 2026
74 of 100 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add salt generation to demo security configuration#6022

Add salt generation to demo security configuration#6022
cwperks merged 10 commits intoopensearch-project:mainfrom
cwperks:demo-salt

cwperks commented Mar 19, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Mar 19, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 28, 2026 •

edited

Loading

Uh oh!

finnegancarroll commented Apr 29, 2026

Uh oh!

cwperks commented Apr 29, 2026

Uh oh!

cwperks commented Apr 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

cwperks commented Apr 30, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cwperks commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Check List

Uh oh!

codecov Bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 1b0b971)

Uh oh!

github-actions Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

finnegancarroll commented Apr 29, 2026

Uh oh!

cwperks commented Apr 29, 2026

Uh oh!

cwperks commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cwperks commented Apr 30, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

github-actions Bot commented Apr 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cwperks commented Mar 19, 2026 •

edited

Loading

codecov Bot commented Mar 19, 2026 •

edited

Loading

github-actions Bot commented Apr 28, 2026 •

edited

Loading

(Review updated until commit `1b0b971`)

github-actions Bot commented Apr 28, 2026 •

edited

Loading

cwperks commented Apr 29, 2026 •

edited

Loading